Deallocate at least #3819
Merged
Deallocate at least #3819
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit e37227e. This broke BitCoin in Microsoft's internal Real World Code (RWC) test suite. They publicy derive an allocator from `std::allocate`, implementing `allocate` and `deallocate` but not `allocate_at_least` (https://github.com/bitcoin/bitcoin/blob/f1b4975461364d5d40d2bfafc6b165dd5d7eec30/src/support/allocators/secure.h#L19-L56). When `vector` allocates memory with `std::allocator::allocate_at_least` and tries to free it with `secure_allocator::deallocate`, terrible things happen. We suspect this pattern is widespread, so we're reverting the change for now.
StephanTLavavej
approved these changes
Jun 22, 2023
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
|
I've pushed a merge with |
|
Thanks for finding the root cause of this major regression and fixing it so fast! 🌲 🎯 🛠️ |
Merged
achow101
added a commit
to bitcoin-core/gui
that referenced
this pull request
Jul 25, 2023
… std::allocator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 9, 2023
…locator 07c59ed Don't derive secure_allocator from std::allocator (Casey Carter) Pull request description: Giving the C++ Standard Committee control of the public interface of your type means they will break it. C++23 adds a new `allocate_at_least` member to `std::allocator`. Very bad things happen when, say, `std::vector` uses `allocate_at_least` from `secure_allocator`'s base to allocate memory which it then tries to free with `secure_allocator::deallocate`. (Discovered by microsoft/STL#3712, which will be reverted by microsoft/STL#3819 before it ships.) ACKs for top commit: jonatack: re-ACK 07c59ed no change since my previous ACK apart from squashing the commits achow101: ACK 07c59ed john-moffett: ACK 07c59ed Reviewed and tested. Performance appears unaffected in my environment. Tree-SHA512: 23606c40414d325f5605a9244d4dd50907fdf5f2fbf70f336accb3a2cb98baa8acd2972f46eab1b7fdec1d28a843a96b06083cd2d09791cda7c90ee218e5bbd5
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#3712 broke BitCoin in Microsoft's internal Real World Code (RWC) test suite. They publicly derive an allocator from
std::allocate, implementingallocateanddeallocatebut notallocate_at_least(https://github.com/bitcoin/bitcoin/blob/f1b4975461364d5d40d2bfafc6b165dd5d7eec30/src/support/allocators/secure.h#L19-L56).
When
vectorallocates memory withstd::allocator::allocate_at_leastand tries to free it withsecure_allocator::deallocate, terrible things happen.We suspect this pattern is widespread, so we're reverting the change for now.
There's a second commit here to reapply the
basic_stringdrive-by fix which we don't want to revert. (We want to merge these commits separately to make it easier to revert the revert in the future.)Fixes VSO-1839141 / AB#1839141.